fix three plugin crash/UB bugs; test coverage for 14 plugins by Alistair-Afton · Pull Request #5939 · DFHack/dfhack

Alistair-Afton · 2026-09-17T15:33:27Z

Summary

3dveins: fix a crash when a tile's biome cannot be resolved (biome_by_idx.at(-1) threw std::out_of_range, which escaped the command handler and terminated the game)
strangemood: fix a crash when --id is given a non-numeric value (std::stoi threw uncaught); invalid ids now return a usage error
suspendmanager: fix an out-of-bounds parameter read when set is given too few arguments
New in-engine regression tests for 3dveins, autodump, changeitem, changelayer, createitem, deramp, export-world-map, filltraffic, flows, liquids, showmood, strangemood, suspendmanager, and tubefill (57 tests)

Test plan

All new tests pass in-game
Full test suite passes (329/330; the one failure was a scratch file not part of this change)

Alistair-Afton · 2026-09-17T17:59:40Z

Root cause of the recurring scripts/overlay/notes:render_existing_notes failure found: NotesOverlay:onRenderFrame() skips drawing map pins while the game is unpaused (in text mode), and the test relies on the ambient pause state set by ensure_fortress. Two tests in this batch (autodump, liquids) forced pause_state = false in their finalizers instead of restoring the prior state, leaving the game unpaused for the notes suite.

Fixed in f42685d: both tests now save/restore pause_state, and suspendmanager's test restores the persisted preventblocking value instead of hardcoding it. Also opened DFHack/scripts#1632 to make the notes test set/restore pause itself so it's robust against similar leaks elsewhere.

SilasD

this is not a full review, I will do that on another day as I am running out of steam today.

in general I don't care about correct code in test suites nearly as much as correct code in tools.

Alistair-Afton · 2026-09-17T22:53:04Z

The Windows empty-config leg failure is root-caused and fixed in a38d479: export-world-map sites (covered by the new test) dereferenced a null civ when a site's owner had an undefined race — a latent crash in develop that no existing test exercised. DF was crashing mid-test (access violation), which the harness records as a failed test; the heap-corruption exit on the second DF instance is the usual quit-time noise.

Fixed by guarding the null civ and actually assigning the intended civ-race fallback. Verified in-game: export-world-map sites exports cleanly and both tests in test/plugins/export-world-map.lua pass.

std::stoi throws uncaught on non-numeric input, crashing the game. Use string_to_int and reject invalid ids with CR_WRONG_USAGE.

…gemood

parameters[1] was read unconditionally when the first parameter was 'set', which is undefined behavior when fewer arguments are given.

…eateitem

MapExtras::Block::biomeIndexAt returns -1 when the biome lookup fails, and biome_by_idx.at(-1) threw std::out_of_range. The exception escaped the plugin command handler and terminated the game. Return NULL like the existing unresolvable-layer path.

Tests must not leak global state to subsequent tests. Restore the prior pause_state instead of forcing unpaused, and restore suspendmanager's persisted preventblocking value instead of hardcoding it.

Trigger real fey/fell/macabre moods per skill and assert the generated job_items match the documented vanilla sets, that BAR and CLOTH demands carry the dimension-corrected quantities (the plugin's vanilla bugfix), and that thread is never requested.

The fallback race lookup dereferenced civ without a null check, so a site whose owner has an undefined race and whose civ_id does not resolve crashed DF (observed as an access violation in CI on the Windows empty-config leg). Assign the civ's race as the intended fallback and only warn when the owner's race is actually undefined.

SilasD

I am satisfied with the Lua test harnesses.

I will be happier if some raw writes are changed to supported API calls such as dfhack.maps.getTileFlags() and dwarfmode.setCursorPos().

I left a very long note in test/plugins/autodump.lua which I DO NOT require be implemented; it's rather blue-sky and not worth holding up a merge. (partly I'm curious how the AI handles it.)

SilasD · 2026-09-18T17:23:11Z

+        expect.eq(y, item.pos.y)
+        expect.eq(z, item.pos.z)
+        expect.false_(item.flags.dump)
+        expect.true_(item.flags.forbid)


TO BE CLEAR, I am NOT insisting that you exercise all or any of these possibliities.

I will NOT hold up a merge over this. any test suite is better than none at all, and I am grateful to have these tests.

this all looks good. another place to test is the map_block the item is in; map_block.items is a sorted vector of item.ids.

it would be best to check that the item ids are ~~removed from the old map_block and~~ inserted into the new map_block on item teleport. including, I suppose, the special case of teleporting to another tile in the same map_block (or even to the item's current tile), which shouldn't change that vector at all.

edit: verifying removal of an item from a map_block is covered below.

edit: it would be good to verify that the map_block.items list remains sorted.

to test this properly, you probably need to find a map_block with a lot of items in it, i.e. #map_block.items has a lot of entries.

edit: there is a tile_occupancy.item flag that should be set on the items' new tile.

edit: item.flags.on_ground should be set, item.flags.in_inventory should be clear, item.flags.in_building should be clear.

SilasD · 2026-09-18T19:10:15Z

+        expect.eq(z, item.pos.z)
+        expect.false_(item.flags.dump)
+        expect.true_(item.flags.forbid)
+    end)


TO BE CLEAR, I am NOT insisting that you exercise all or any of these possibliities.

I will NOT hold up a merge over this. any test suite is better than none at all, and I am grateful to have these tests.

this test suite doesn't have anything that verifies the item is removed from the old location.

fully exercising that will be rather hard, as there are ~~three~~ four common cases:

the item was on the floor.

items with item.flags.construction set should not be teleported.

items with item.flags.artifact set should not be teleported. (I am in favor of changing the code to allow it, but that's a different discussion.)

items with items.flag.forbid set should only be teleported if the forbidden option is given. and conversely, items with items.flags.forbid clear should not be teleported if the forbidden option is given.

the item id should be removed from map_block.items unless the item's new tile is in the same map_block.

the tile's tile_occupancy.item flag should be cleared if there are no more items on that tile. I think the best way to determine this is by walking all item ids still in the map_block.items vector, getting each item, and testing if the item.pos matches the tile's pos. if there are any that match, that flag should remain set.

the map_block.items vector should still be sorted.

the item was in a building.

item.pos may not be valid for items in buildings, use dfhack.items.getPosition(item).

items which are in a building and can be removed from that building have

item.flags.on_ground == false

item.flags.in_inventory == false

item.flags.in_building == false

the item has a general_ref_building_holderst with the building's id.

items which are part of a building, i.e. item.flags.in_building == true, should not be teleported.

a building's contained_items vector is an unsorted list of buildingitemst which point to the items inside it.
after the teleport:

all item data should be set per your current tests and the previous comment.

the item should no longer have a general_ref_building_holderst.

the relevant element of the building.contained_items vector should have been removed.

cages are a special case:

items in a cage are contained in the cage item, not the cage building.

the cage building only has the cage item and and attached mechanisms.

I think animal traps work the same way for both the bait item and the vermin item, but I am not certain.

the item was in a container.

item.pos may not be valid for items in containers, use dfhack.items.getPosition(item).

items in a container have item.flags.in_inventory == true, item.flags.on_ground == false, item.flags.in_building == false.

items in a container have a general_ref_contained_in_itemst with the container's item id.

the container has a general_ref_contains_itemst with the item's id.

general_refs vectors are unsorted.
after the teleport:

all item data should be set per your current tests and the previous comment.

the item should no longer have a general_ref_contained_in_itemst.

the container should no longer have a general_ref_contains_itemst with that item's id.

either the container weight should have changed or flags.weight_computed should be clear? don't bother.

the item was in a unit's inventory.

item.pos may not be valid for items in in a unit's inventory, use dfhack.items.getPosition(item).

items in a unit's inventory have item.flags.in_inventory == true, item.flags.on_ground == false, item.flags.in_building == false, exactly like items in a container.

items in a unit's inventory have a general_ref_unit_holderst with the unit's unit id.

the unit.inventory vector has a unit_inventory_item that points to the item.
after the teleport:

all item data should be set per your current tests and the previous comment.

the item should no longer have a general_ref_unit_holderst.

the relevant element of the unit.inventory vector should have been removed.

the unit's weight should somehow be flagged as need-to-recompute? don't bother.

there are also rare cases: the item is/was a projectile, the item is uncategorized, possibly others I can't think of. I think these don't need to be tested, as I am not convinced that autodump should even attempt to handle them.

this is all really messy and may not need to be verified for autodump, because autodump uses library calls that handle the messiness. OTOH I don't think we have anything else that verifies those calls.

all of these tests will not be necessary for the destroy-item case, because that relies on the game doing the work.

this got big. please do not feel any obligation here.

SilasD · 2026-09-18T19:13:38Z

+local function set_cursor(x, y, z)
+    df.global.cursor:assign{x=x, y=y, z=z}
+end
+
+local function clear_cursor()
+    df.global.cursor:assign{x=-30000, y=-30000, z=-30000}
+end


API dwarfmode.setCursorPos and friends per the updates for test/plugins/autodump.lua.

updating is preferable but not necessary.

SilasD · 2026-09-18T19:19:08Z

+        df.global.cursor:assign{x=-30000, y=-30000, z=-30000}
+    end, function()
+        df.global.cursor:assign{x=-30000, y=-30000, z=-30000}


API dwarfmode.setCursorPos and friends per the updates for test/plugins/autodump.lua.

SilasD · 2026-09-18T19:21:58Z

+local function count_items_at(x, y, z)
+    local n = 0
+    for _, item in ipairs(df.global.world.items.other.IN_PLAY) do
+        if item.pos.x == x and item.pos.y == y and item.pos.z == z then


item.pos is unreliable; better to use API dfhack.items.getPosition(item)

SilasD · 2026-09-18T20:30:11Z

+        expect.eq(before + 1, count_items_at(x, y, z))
+
+        for _, item in ipairs(df.global.world.items.other.IN_PLAY) do
+            if item.pos.x == x and item.pos.y == y and item.pos.z == z


item.pos is unreliable; better to use API dfhack.items.getPosition(item)

SilasD · 2026-09-18T20:32:52Z

+end
+
+local function des_at(x, y, z)
+    return dfhack.maps.getTileBlock(x, y, z).designation[x % 16][y % 16]


it would probably be better to use dfhack.maps.getTileFlags()
untested code:
return select(1, dfhack.maps.getTileFlags(x, y, z))

SilasD · 2026-09-18T20:52:26Z

+-- every item_type a fey/secretive/possessed mood may ever request
+-- (bars/wafers and cloth are the only dimensional demands; thread is
+-- never requested)
+local ALLOWED_ITEM_TYPES = {
+    BOULDER=true, WOOD=true, SKIN_TANNED=true, CLOTH=true, BAR=true,
+    ROUGH=true, SMALLGEM=true, BLOCKS=true, NONE=true,
+}
+


Edit: I will NOT delay merging this PR over this request.

you're correct that THREAD is never requested, but I would like you to add it anyway.

Rationale 1: PR #5952 is likely to be merged, and it has code to set a field for THREAD.

Rationale 2: this recent conversation on our Discord mentions that including THREAD is harmless.

Quietust — Yesterday at 1:54 PM
The full set of strange mood item requests are as follows:
Fell: CORPSE
Macabre: REMAINS, CORPSEPIECE of BONE, CORPSEPIECE of SKULL
Other: BOULDER, WOOD, SKIN_TANNED, CLOTH, BAR, ROUGH, CORPSEPIECE of BONE or SHELL
Decorations: REMAINS (macabre only), CORPSEPIECE of BONE, WOOD, BAR, SMALLGEM, BLOCKS, ROUGH, BOULDER, SKIN_TANNED, CLOTH

Which reminds me - there were recent changes to strange moods, and while I'm pretty sure those were restricted to outputs rather than inputs, I really should recheck the mood startup code to make sure the DFHack plugin is still correct.
And I also need to add a comment somewhere in there (ideally at the very top) to indicate which version I last checked.
I'm sure this plugin is one that would probably be eligible for Lua conversion, but that will make it far more difficult for me to maintain.
(and given that I'm probably the primary maintainer of that plugin, I don't know whether it would be worth it in the long run)

Dikbutdagrate — Yesterday at 4:03 PM
I was kind of hoping there were additional tweaks to strange moods, which Tarn perhaps maybe neglected to mention with the change to mood item outputs in 53.16 (code is ancient, and could use some spice). Haven't noticed anything yet though.

rome of oxtrot — Yesterday at 4:26 PM
it's harmless to include it and there's always the (small) chance that toady will, intentionally or otherwise, add thread as a possible rquested material

Quietust — Yesterday at 5:05 PM
Funny thing: in the code for adding mood decoration material requests, there's code to set min_dimension for BAR and CLOTH and it also handles THREAD.

SilasD · 2026-09-18T20:57:50Z

+    elseif item.item_type == df.item_type.CLOTH then
+        expect.eq(10000, item.min_dimension)
+        expect.eq(0, item.quantity % 10000)
+    else


if you add THREAD, check its expected item.min_dimension == 15000 here, and the item.quantity as well.

SilasD · 2026-09-18T20:59:28Z

+                'base item for ' .. skill)
+            check_dimension(base)
+            for _, item in ipairs(job.job_items.elements) do
+                expect.ne(df.item_type.THREAD, item.item_type)


if you add THREAD, remove this test.

Vanilla's mood decoration request code handles THREAD with a 15000 min_dimension like BAR/CLOTH, and it is a plausible future or rare request type, so allow it in the allowed-types set, assert its dimensional fields, and drop the never-requested assertion.

Alistair-Afton · 2026-09-19T13:28:54Z

THREAD changes done in ef1c305:

added THREAD to ALLOWED_ITEM_TYPES (comment updated to match)
check_dimension now asserts min_dimension == 15000 and quantity % 15000 == 0 for THREAD items
removed the expect.ne(df.item_type.THREAD, ...) assertion

Per review: replace raw df.global.cursor writes with dwarfmode.setCursorPos/clearCursorPos, read tile flags via dfhack.maps.getTileFlags, and resolve item positions with dfhack.items.getPosition instead of item.pos. Also strengthen checks: deramp now verifies the z+1 tile is cleared only when it was RampTop (with a fabricated non-RampTop case), and autodump verifies ground/inventory/building flags, occupancy.item on the destination tile, and map_block.items membership/sortedness.

Alistair-Afton · 2026-09-19T13:39:38Z

All review comments addressed in 1f64b0d (plus ef1c305 for the strangemood THREAD items):

dwarfmode.setCursorPos/clearCursorPos now used in changeitem, changelayer, createitem, filltraffic, liquids (autodump already had it)
dfhack.maps.getTileFlags replaces getTileBlock().designation[...] in liquids
dfhack.items.getPosition replaces item.pos reads in createitem (both sites)
deramp: now checks the z+1 tile — RampTop must become OpenSpace, anything else must be untouched; leaves_non_ramptop_above fabricates the second case
autodump: dump_moves_item_to_cursor now also asserts on_ground/in_inventory/in_building flags, tile_occupancy.item on the destination tile, item-id membership in the new map_block.items, removal from the old block, and that the vector stays sorted
strangemood: THREAD added to the allowed set with min_dimension == 15000/quantity checks, never-requested assertion removed

Verified in-game: all 44 tests across the touched files pass (441 checks). The four removal-verification cases in the longer autodump note (building/container/unit-inventory refs) are not implemented — happy to take those as a follow-up if you want them.

Also cleaned up stale test pollution in the fort: two units had leftover mood state from an earlier interrupted test run (one with a StrangeMoodForge job and only a UNIT_WORKER ref), which was making all strangemood --force invocations fail.

Alistair-Afton force-pushed the test-plugin-coverage-10 branch 2 times, most recently from f3c1610 to 6e64b3a Compare September 17, 2026 15:50

Alistair-Afton changed the title ~~fix strangemood --id crash; test coverage for autodump, filltraffic, export-world-map, strangemood~~ fix strangemood/suspendmanager input validation; test coverage for 9 more plugins Sep 17, 2026

Alistair-Afton changed the title ~~fix strangemood/suspendmanager input validation; test coverage for 9 more plugins~~ fix strangemood/suspendmanager input validation; test coverage for 14 more plugins Sep 17, 2026

Alistair-Afton changed the title ~~fix strangemood/suspendmanager input validation; test coverage for 14 more plugins~~ fix three plugin crash/UB bugs; test coverage for 14 plugins Sep 17, 2026

Alistair-Afton mentioned this pull request Sep 17, 2026

core: report plugin command exceptions instead of crashing the game #5940

Open

3 tasks

Alistair-Afton force-pushed the test-plugin-coverage-10 branch from 583dfb2 to 14ccd5b Compare September 17, 2026 17:22

Alistair-Afton mentioned this pull request Sep 17, 2026

notes test: pause the game while the overlay is installed DFHack/scripts#1632

Open

1 task

SilasD requested changes Sep 17, 2026

View reviewed changes

Comment thread test/plugins/autodump.lua Outdated

Comment thread test/plugins/autodump.lua Outdated

Comment thread test/plugins/liquids.lua Outdated

Alistair-Afton force-pushed the test-plugin-coverage-10 branch from 934c663 to 610c05d Compare September 17, 2026 21:36

SilasD reviewed Sep 17, 2026

View reviewed changes

Comment thread test/plugins/autodump.lua

Alistair-Afton added 12 commits September 18, 2026 08:47

fix strangemood crash on non-numeric --id value

0a192a7

std::stoi throws uncaught on non-numeric input, crashing the game. Use string_to_int and reject invalid ids with CR_WRONG_USAGE.

test: add coverage for autodump, filltraffic, export-world-map, stran…

1db1882

…gemood

suspendmanager: fix out-of-bounds read on bare 'set'

1ae50a0

parameters[1] was read unconditionally when the first parameter was 'set', which is undefined behavior when fewer arguments are given.

test: add coverage for liquids, tubefill, suspendmanager, 3dveins, cr…

d4e1c76

…eateitem

test: add coverage for flows, showmood, deramp, changelayer, changeitem

079d8b8

restore global state in test cleanup

3c580f0

Tests must not leak global state to subsequent tests. Restore the prior pause_state instead of forcing unpaused, and restore suspendmanager's persisted preventblocking value instead of hardcoding it.

use dfhack.world pause API instead of raw pause_state

05b8137

strangemood test: mirror STRANGE_MOODS caste check in unit picker

cc0ef10

autodump test: use gui.dwarfmode cursor helpers

f8351ea

Alistair-Afton force-pushed the test-plugin-coverage-10 branch from e672c73 to f8351ea Compare September 18, 2026 06:47

remove duplicated timestream changelog entry

d310a26

SilasD approved these changes Sep 18, 2026

View reviewed changes

ab9rf added 2 commits September 19, 2026 17:05

Merge branch 'develop' into test-plugin-coverage-10

85327b7

Merge branch 'develop' into test-plugin-coverage-10

6d4be6f

Conversation

Alistair-Afton commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Alistair-Afton commented Sep 17, 2026

Uh oh!

SilasD left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Alistair-Afton commented Sep 17, 2026

Uh oh!

Uh oh!

SilasD left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Alistair-Afton commented Sep 19, 2026

Uh oh!

Alistair-Afton commented Sep 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Alistair-Afton commented Sep 17, 2026 •

edited

Loading